-
Notifications
You must be signed in to change notification settings - Fork 7
HTML-encode the JSON response based on the content type #7385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
| @Override | ||
| public void writeProperty(String name, Object value) throws IOException | ||
| { | ||
| super.writeProperty(sendHtmlJsonResponse ? PageFlowUtil.filter(name) : name, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a comment here to the effect that super.writeProperty() calls writeObject() which encodes
| } | ||
| else | ||
| { | ||
| super.writeObject(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So all non-String values are safe to render without encoding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, yes. But there are other possible values that could end up rendering as strings. I was able to change the override approach to catch more of those theoretical pathways. I didn't see a way to intercept this line though:
else if (isSerializeViaJacksonAnnotations() || value instanceof SimpleResponse<?>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If everything is supposed to be encoded, could this be tackled from the stream side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If everything is supposed to be encoded, could this be tackled from the stream side?
Rationale
A certain combination of HTTP headers can cause mixup in terms of
Content-Typeand encoding. I don't think this will happen in real-world scenarios, but can be hit by security scanners.The reported scenario is doing a POST to
query-importwithout theX-Requested-With=XMLHttpRequestHTTP request header. The actual form submission sends that header, so the server responds withapplication/json. Without the header, the server returns JSON with astext/htmlbut fails to HTML encode.Changes
Tasks 📍